Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat #140] 채팅 요청 목록 조회 API #141

Merged
merged 14 commits into from
Nov 15, 2024
Merged

Conversation

hyun2371
Copy link
Member

관련 이슈

📑 작업 상세 내용

image image
  • 요청 주체에 따라 요청중 상태의 표시 UI가 다름 -> 요청대기, 수락하기
    • 요청 목록에 isInquirer, chatStatus 필드 추가
  • 채팅 목록 조회에 불필요한 chatStatus 필드 제거

💫 작업 요약

  • 기존) 상태 파라미터에 따라 하나의 API 조회했던 채팅 목록, 요청 목록
  • 변화) 각각의 API로 쪼개기

🔍 중점적으로 리뷰 할 부분

  • 비즈니스 로직 중복되는 코드 어떻게 메서드로 추출할지 고민해보겠습니다.
  • 추후 채팅방 목록 조회에 채팅방 별 안 읽은 메시지 수를 추가할 예정입니다.

@hyun2371 hyun2371 added the ✨ feat 기능 추가 label Nov 15, 2024
@hyun2371 hyun2371 requested a review from dudxo November 15, 2024 04:45
@hyun2371 hyun2371 self-assigned this Nov 15, 2024
@hyun2371 hyun2371 linked an issue Nov 15, 2024 that may be closed by this pull request
1 task
Copy link

Code Coverage

Overall Project 85.71% 🍏
Files changed 100% 🍏

File Coverage
ChatRoomController.java 100% 🍏
ChatRoomService.java 94.57% 🍏
ChatRoomQueryRepositoryImpl.java 88.72% 🍏
ChatStatus.java 50% 🍏

Copy link

Test Results

 25 files   25 suites   14s ⏱️
129 tests 129 ✅ 0 💤 0 ❌
130 runs  130 ✅ 0 💤 0 ❌

Results for commit 445c947.

Copy link
Collaborator

@dudxo dudxo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크럼 때 말씀해주신 부분을 살펴보았는데 바로 생각나진 않네요..!

고생하셨습니다.

.toList();

List<LatestChatMessage> latestChatMessages
= chatMessageQueryRepository.findLatestChatByChatRoomIds(chatRoomIds);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

�가장 최신의 채팅 메시지 목록을 가져오는 메서드는 분리를 통해 중복을 줄일 수 있을 것 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getChatPartner()를 리팩토링해도 괜찮겠네요.

if문에서 바로 return을 던져주고 있기 때문에 else과 else if문을 사용을 줄여도 좋을 것 같아요

private Member getChatPartner(Long memberId, ChatRoom chatRoom) {
		if (Objects.equals(chatRoom.getAnswerer().getId(), memberId)) {
			return chatRoom.getInquirer();
		}
		if (Objects.equals(chatRoom.getInquirer().getId(), memberId)) {
			return chatRoom.getAnswerer();
		}
		throw new ValidationException(ChatErrorCode.UNAUTHORIZED_CHAT_ROOM);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아니면 더 좋게 바꿀 수 도 있을 것 같아요. Member 객체에서 memberId만 getter통해 가져오기 보다는 Member 객체가 스스로 일을 하도록 하는거죠.

public class Member {
          private Long memberId;
          ....

          public boolean isSameId(Long otherId) {
                     return Objects.equals(otherId);
          }
}

private Member getChatPartner(Member member, ChatRoom chatRoom) {
		if (member.isSameId(chatRoom.getAnswerer()) {
			return chatRoom.getInquirer();
		}
		if (member.isSameId(chatRoom.getInquirer()))) {
			return chatRoom.getAnswerer();
		}
		throw new ValidationException(ChatErrorCode.UNAUTHORIZED_CHAT_ROOM);
}

Copy link
Member Author

@hyun2371 hyun2371 Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵. 제안주신 부분 다음 PR에 참고해서 반영하겠습니다! 감사합니다

@hyun2371 hyun2371 merged commit 96c5266 into dev Nov 15, 2024
3 checks passed
@hyun2371 hyun2371 deleted the feat/#140/chat-proposal-list branch November 15, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feat 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ 채팅 요청 목록 API
2 participants